Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
lan17
left a comment
lan17
left a comment
There was a problem hiding this comment.
I like the direction here, but I don't think this base PR is ready to merge yet. The main thing I'd fix first is the enabled= override becoming sticky process state. I also think we need an explicit lifecycle/ownership story for external sinks before we merge the base abstraction.
|
|
||
| # Check if enabled | ||
| is_enabled = enabled if enabled is not None else get_settings().observability_enabled | ||
| if enabled is not None: |
There was a problem hiding this comment.
This changes enabled= from a per-call override into a process-wide mutation. After one init(..., observability_enabled=False), later init/re-init flows and later sink resolution stay disabled until something explicitly rewrites settings. I don't think we want an init-time override to poison the rest of the process like that.
| """Register an external control-event sink. | ||
|
|
||
| Registered sinks receive the same finalized control-event payloads emitted | ||
| through the SDK's local, server, and merged event flows. When one or more |
There was a problem hiding this comment.
Once we expose caller-registered sinks here, we also need a lifecycle story for them. Right now shutdown still only drains the built-in batcher, so any buffered or networked external sink can still drop data on process exit. I'd rather make ownership explicit here before we merge the base API.
2 participants
Summary
Added a vendor-neutral external control-event sink registration API in the Python SDK so integrations can receive finalized
ControlExecutionEventpayloads without adding vendor-specific logic to OSS.Updated observability delivery so
observability_enabledis the master gate, and when enabled the SDK uses registered sinks if present, otherwise the existing default OSS batcher sink.Added docs and test coverage for sink registration, override behavior, fallback to default delivery, and local/server/merged event delivery parity.
Scope
User-facing/API changes:
register_control_event_sink(...),unregister_control_event_sink(...), andget_registered_control_event_sinks(...)to the Python SDK public APIInternal changes:
observability_enabledbefore any sink delivery occursOut of scope:
Risk and Rollout
Risk level: medium
Rollback plan: Revert the SDK observability sink-resolution changes and public sink registration API additions in the Python SDK. Since default OSS delivery remains the fallback when no external sink is registered, rollback is isolated to the SDK observability layer.
Testing
make check— focused validation was run instead. Full workspace checks were unnecessary for this scoped SDK change. There is also a pre-existing unrelated SDK typecheck issue aroundgoogle.adkuntyped imports.Checklist